Can you wrap all these details on the error paths in StoreAssetPairStore? That way we don't need to have this.

The whole direction here was the opposite of this change request.
The ideology I'm following is that functions return the error they encounter, without their own context.
If you call a go standard library function, you'll get the same thing.
It's down to the caller to decide if it wants to give context to that error, and what that context looks like from it's perspective.
From the perspective of the function caller, they know what they called, and so the error returned should be undecorated.

Case in point: StoreAssetPairStore is not default asset formats so these callers all needed to clarify the context they were calling that function in.

This is why you'll often see me decorate errors one level above where GCT has previously been doing it.
Sometimes it can feel slightly weighty, in this example where we copy the same context everywhere.

We could definitely add a common error for errStoringDefaultAssetFormats though, and I think that's warranted.
I'll circle back to this after the others and see if you've replied :)

Adding one more bit of context:
This implies that functions must also add their own context.
e.g. If StoreAssetPairStore encountered some error interacting with a database, like "Record not found", We'd expect that error to come without context.
So StoreAssetPairStore should add the context error searching for glove size in database: %w.

From an implementation perspective, maintaining this standard can introduce toil during implementation and reviews, especially since any failure in SetDefaults would inherently cause the exchange to malfunction. Differentiating between errors in this context seems redundant. Still suggest wrapping asset and exchange for the exchange method.
But I will leave this open for other peoples perspective. @thrasher- @gloriousCode

Looking at what both of you saying, I don't think there should be a hard rule about either. There will be times I will absolutely argue for errors returned to contain the data which it is complaining about + some extra context tidbits. Things like "they know what they called" in a large, semi-automated system like GCT doesn't work as well as you'd hope and I wouldn't want that universally applied.

Another issue which depends on the situation is the onus on the caller, so for some functions that would mean decorating the returned error in a large amount of areas of code (which you have done in 11 places) could have just been done in the function once.

Like Shazbert says, the onus is then put on the reviewer to say "hey decorate that error" when a new call is added. If everything relies on reviewers catching people not doing things on top of all the things they are doing, then we are going to get unhelpful, undecorated errors. An unlabelled asset is invalid in the logs is useless which is why we do what we do.

Given that this function is only called within SetDefaults() and gk has copy pasted the same message everywhere, I'm okay with it staying - even if I find it strange to advocate for copy and pasting strings in 11 places. You won't be able to use this text to get out of all future instances 😄

That's an interesting point about how GCT is a system.
I'd be taking a completely different perspective if GCT was more closed, and the paths to these functions were all private, and callers knew to log it without decoration.
Also: logged with double decoration is better than logged with no context. That's an argument against my change.

Looking at what both of you saying, I don't think there should be a hard rule about either. There will be times I will absolutely argue for errors returned to contain the data which it is complaining about + some extra context tidbits. Things like "they know what they called" in a large, semi-automated system like GCT doesn't work as well as you'd hope and I wouldn't want that universally applied.

Another issue which depends on the situation is the onus on the caller, so for some functions that would mean decorating the returned error in a large amount of areas of code (which you have done in 11 places) could have just been done in the function once.

Like Shazbert says, the onus is then put on the reviewer to say "hey decorate that error" when a new call is added. If everything relies on reviewers catching people not doing things on top of all the things they are doing, then we are going to get unhelpful, undecorated errors. An unlabelled asset is invalid in the logs is useless which is why we do what we do.

Given that this function is only called within SetDefaults() and gk has copy pasted the same message everywhere, I'm okay with it staying - even if I find it strange to advocate for copy and pasting strings in 11 places. You won't be able to use this text to get out of all future instances 😄